fix: retain recently written external resources missing from a stale update - #3565
fix: retain recently written external resources missing from a stale update#3565csviri wants to merge 1 commit into
Conversation
…update An update of the whole resource set of a primary (a poll result or a received event) might have been created before the reconciler wrote a resource, thus not containing it yet. Since such updates are handled as the full actual state, the write was lost from the cache, and the next reconciliation created a duplicate of an already created resource or repeated an already executed update. Writes are now marked as unconfirmed and retained for the next update if it either does not contain the resource at all - the expected case for a create - or still contains the state that the write replaced. Any other state is treated as a change made outside of the reconciler and accepted as actual. Marks are dropped on the first update, so a resource really deleted or changed meanwhile is not retained indefinitely. Also guards handleRecentResourceUpdate against a missing cache entry, and resolves the actual resources from the state resources in the external state bulk dependent integration test, which is the recommended approach for resources that take longer to become visible.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves external-resource cache consistency when full-state updates are stale relative to reconciler writes.
Changes:
- Retains recently written resources during stale cache refreshes.
- Adds regression tests for stale and externally changed resources.
- Updates external-state integration behavior and documentation.
A critical issue remains: consecutive writes to the same resource can overwrite prior unconfirmed state, allowing a stale update to replace the latest value and trigger repeated reconciliation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Summary |
|---|---|
operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/externalstate/externalstatebulkdependent/BulkDependentResourceExternalWithState.java |
Resolves resources through persisted state. |
operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSourceTest.java |
Adds cache consistency regression tests. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java |
Implements unconfirmed-write retention. |
docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md |
Documents external-state consistency guidance. |
Suppressed comments (3)
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:84
- These marks are not cleared when a primary is deleted through
CachingInboundEventSource: itsonResourceDeletedonly removesfetchedForPrimariesand never callshandleDeleteto remove this map. If the sameResourceIDis later recreated, the first full update can satisfynewResource == nulland reinsert the old written resource into the new primary's cache. Tie this state to primary deletion (and clear the corresponding cache) in that lifecycle path.
private final Map<ResourceID, Map<ID, RecentWrite<R>>> unconfirmedWrites =
new ConcurrentHashMap<>();
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:219
- This branch also retains an updated resource when
newResource.equals(write.replaced()), so the log message is inaccurate for that common stale-update case: the resource is present, but its new state is not reflected. Please use wording such as "not reflected in the update" so debug logs do not misdiagnose retained updates.
"Retaining recently written resource missing from the update. Primary ID: {},"
+ " resource ID: {}",
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java:298
- The null check is a stated behavior change, but the added tests only call
handleRecentResourceUpdateafter seeding the primary/resource in the cache. Please add a regression test for an absent primary or resource entry that verifies this path does not throw and does not cache the update.
if (actualResource != null && actualResource.equals(previousVersionOfResource)) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| private void markUnconfirmedWrite(ResourceID primaryID, ID resourceId, RecentWrite<R> write) { | ||
| unconfirmedWrites.computeIfAbsent(primaryID, id -> new HashMap<>()).put(resourceId, write); |
An update of the whole resource set of a primary (a poll result or a received
event) might have been created before the reconciler wrote a resource, thus not
containing it yet. Since such updates are handled as the full actual state, the
write was lost from the cache, and the next reconciliation created a duplicate
of an already created resource or repeated an already executed update.
Writes are now marked as unconfirmed and retained for the next update if it
either does not contain the resource at all - the expected case for a create -
or still contains the state that the write replaced. Any other state is treated
as a change made outside of the reconciler and accepted as actual. Marks are
dropped on the first update, so a resource really deleted or changed meanwhile
is not retained indefinitely.
Also guards handleRecentResourceUpdate against a missing cache entry, and
resolves the actual resources from the state resources in the external state
bulk dependent integration test, which is the recommended approach for
resources that take longer to become visible.